Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Create ELF Fieldset - Stage 2 Proposal #1294

Merged
merged 13 commits into from
May 4, 2021
Merged

Conversation

peasead
Copy link
Contributor

@peasead peasead commented Mar 8, 2021

  • Have you signed the contributor license agreement? Yes
  • Have you followed the contributor guidelines? Yes
  • For proposing substantial changes or additions to the schema, have you reviewed the RFC process? Yes
  • If submitting code/script changes, have you verified all tests pass locally using make test? Yes
  • If submitting schema/fields updates, have you generated new artifacts by running make and committed those changes? Yes
  • Is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed. Yes
  • Have you added an entry to the CHANGELOG.next.md? Yes

Preview of markdown proposal

@peasead peasead added the RFC label Mar 8, 2021
@peasead peasead self-assigned this Mar 8, 2021
@peasead peasead changed the title [RFC] Create ELF - Stage 2 [RFC] Create ELF Fieldset - Stage 2 Proposal Mar 8, 2021
rfcs/text/0015-create-file-elf.md Outdated Show resolved Hide resolved
rfcs/text/0015-create-file-elf.md Outdated Show resolved Hide resolved
@ebeahan ebeahan requested a review from devonakerr March 15, 2021 21:31
devonakerr
devonakerr previously approved these changes Mar 16, 2021
Copy link

@devonakerr devonakerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still the outstanding concern of how to best capture the expected structure of elf.imports.

Defining elf.imports as flattened is straight-forward:

PUT example
{
  "mappings": {
    "properties": {
      "elf": {
        "properties": {
          "imports": {
            "type": "flattened"
          }
        }
      }
    }
  }
}

The outstanding item is what to do about elf.imports.name and elf.imports.type. We cannot explicitly define them underneath imports in the index; Elasticsearch will throw an exception:

"reason" : "unknown parameter [properties] on mapper [imports] of type [flattened]"

We've taken an approach before with objects, like dns.answer, where the docs have the top-level dns.answer field and the expected sub-keys (like dns.answers.class) listed as separate fields. I don't think that approach makes sense for flattened though since there are not actual field mappings associated with each sub-field.

I think our best approach would be describing the expected structure of elf.imports in the field's description. Something like

description: >
  description text....
  
  `elf.imports` expects the following object structure:
  
  {
    "imports": {
      "name": "value",
      "type": "value"
    }
  }

Whatever's decided, we need to add elf.imports to the field definitions. I also made a couple of other notes.

rfcs/text/0015-create-file-elf.md Outdated Show resolved Hide resolved
rfcs/text/0015/elf.yml Show resolved Hide resolved
@peasead
Copy link
Contributor Author

peasead commented Mar 25, 2021

There's still the outstanding concern of how to best capture the expected structure of elf.imports.

Defining elf.imports as flattened is straight-forward:

PUT example
{
  "mappings": {
    "properties": {
      "elf": {
        "properties": {
          "imports": {
            "type": "flattened"
          }
        }
      }
    }
  }
}

The outstanding item is what to do about elf.imports.name and elf.imports.type. We cannot explicitly define them underneath imports in the index; Elasticsearch will throw an exception:

"reason" : "unknown parameter [properties] on mapper [imports] of type [flattened]"

We've taken an approach before with objects, like dns.answer, where the docs have the top-level dns.answer field and the expected sub-keys (like dns.answers.class) listed as separate fields. I don't think that approach makes sense for flattened though since there are not actual field mappings associated with each sub-field.

I think our best approach would be describing the expected structure of elf.imports in the field's description. Something like

description: >
  description text....
  
  `elf.imports` expects the following object structure:
  
  {
    "imports": {
      "name": "value",
      "type": "value"
    }
  }

Whatever's decided, we need to add elf.imports to the field definitions. I also made a couple of other notes.

I just addressed this, please LMK if that meets your concern.

@peasead peasead requested review from dcode, devonakerr and ebeahan March 29, 2021 16:35
devonakerr
devonakerr previously approved these changes Mar 29, 2021
Copy link

@devonakerr devonakerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Andy, thanks for taking some time to address my questions offline and thanks to both yourself and Eric for your diligence with this RFC. Defining name and type field values should be sufficient to make the elf.imports mappings usable, without going too deeply into the weeds.

@peasead
Copy link
Contributor Author

peasead commented Apr 5, 2021

Just pinging to get a 2/2 review.

Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay.

Spotted a few items on a final read-through, but otherwise everything looks good to me.

rfcs/text/0015/elf.yml Show resolved Hide resolved
rfcs/text/0015/elf.yml Outdated Show resolved Hide resolved
rfcs/text/0015-create-file-elf.md Outdated Show resolved Hide resolved
@peasead peasead requested review from devonakerr and ebeahan April 9, 2021 05:35
Copy link

@devonakerr devonakerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@peasead peasead merged commit 65998e2 into master May 4, 2021
@peasead peasead deleted the create-elf-stage-2 branch May 17, 2021 20:44
@ebeahan
Copy link
Member

ebeahan commented Nov 30, 2021

@devonakerr @peasead @dcode I know we paused on the work around the elf.* fields. However, since the elf.* RFC did make it to stage 2, the fields are now published in the docs. I prefer to avoid indefinitely leaving the elf.* fields at a beta status (and as time goes on, users will adopt them, beta or not 😅 ).

Do we feel comfortable with how these fields are defined now? Are there any reservations for moving to stage 3 and having these fields moving from beta to GA in the schema?

@peasead
Copy link
Contributor Author

peasead commented Dec 1, 2021

I don't see any outstanding concerns in this Issue; so we can probably move this to Stage 3.

@peasead
Copy link
Contributor Author

peasead commented Dec 1, 2021

Is there a new sponsor or would you like me to move this?

@ebeahan
Copy link
Member

ebeahan commented Dec 7, 2021

@peasead if you're okay opening the stage 3 PR and if @devonakerr is okay with continuing to sponsor, that'd be great!

@devonakerr
Copy link

I heed the call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants